fix: correct extended explain operator counts for reuse wrappers and CometSubqueryBroadcast - #5206
Open
andygrove wants to merge 4 commits into
Open
fix: correct extended explain operator counts for reuse wrappers and CometSubqueryBroadcast#5206andygrove wants to merge 4 commits into
andygrove wants to merge 4 commits into
Conversation
…ed explain The extended explain summary reported operator coverage and transition count but said nothing about expressions, so there was no way to see how much of a plan's expression evaluation ran in native DataFusion kernels versus Spark's own generated code inside the codegen dispatcher. Track both paths on the expression tree with a new `NATIVE_EXPRS` tag, the counterpart of the existing `CODEGEN_DISPATCH_EXPRS`, and roll them up per operator onto the converted Comet plan node alongside the info messages. The summary line now ends with, for example: Comet accelerated 14 expressions (14 native, 1 codegen dispatch). Counts are distinct expression names per plan. A name can appear in both buckets when the same function is lowered natively for one set of arguments and dispatched for another, so the total is the union rather than the sum. Structural nodes (attribute references, literals, aliases, bound references) are excluded; they appear in nearly every tree and would swamp the names that matter, and excluding literals also keeps the dispatcher's closure-serialized payload out of the native count. Add `ExtendedExplainInfo.getNativeExpressions` and `getCodegenDispatchExpressions` for programmatic access to the two name lists. Dispatch tagging is no longer gated on `spark.comet.explain.codegen.enabled`; that config now controls only whether the `[COMET-INFO: JVM codegen dispatcher: ...]` segment is rendered, so the counts are available by default. Enable that config in `CometPlanStabilitySuite` so the TPC-DS goldens record which expressions took the dispatcher path, not just how many. Golden files regenerated for Spark 3.4, 3.5, 4.0, 4.1 and 4.2; the only changes are summary lines and `COMET-INFO` segments.
Quality pass over the previous commit, no behavior change:
- Collapse the three byte-identical "append a name to a Set-valued tag" bodies
(`withInfo`, `withCodegenDispatchExpr`, `withNativeExpr`) onto one private
`appendTagValue`, with a bulk `appendTagValues` for lifting a whole name set.
- Add `CometExplainInfo.collectTagValues` and use it from both roll-up sites in
`CometExecRule` (including the pre-existing `EXTENSION_INFO` line) and from
`liftCoverageTags`, replacing three spellings of the same union.
- `rollUpInfoMessages` now writes the coverage tags through `appendTagValues`
rather than raw `setTagValue`, so there is one way to write these tags.
- `liftCoverageTags` walks the promoted tree once instead of twice, and drops
the identity `collect { case e: Expression => e }`.
- Classify native-vs-dispatched from a dedicated `DISPATCHED_SELF` marker
instead of sniffing `CODEGEN_DISPATCH_EXPRS`. That tag accumulates descendant
names during roll-up and lifting, so reading it as "was I dispatched" made the
classification depend on planner visit order.
- Route the two accessors through the existing `sortup` traversal, as
`fallbackReasons` already does, so they no longer render and discard a full
plan string just to read tags off nodes.
- Drop two no-op configs from the new test, fix an inaccurate comment about the
dispatcher's closure payload (that `Literal` is built locally and is never
reachable from `op.expressions`), and trim a comment that restated its code.
The summary read Comet accelerated 13 expressions (13 native, 1 codegen dispatch). The total was the union of the two name sets rather than their sum, because the same function can be lowered natively for one set of arguments and dispatched for another, so a name can be in both. Correct by design, but it presents as an arithmetic error to anyone reading the line. It now reads Accelerated expressions: 13 native, 1 codegen dispatch. which reports the same two counts, has no total to misread, and is grammatical at any count. Also drops the merged-set allocation the total needed. Golden files regenerated for Spark 3.4, 3.5, 4.0, 4.1 and 4.2; the only changed lines are the summary.
…CometSubqueryBroadcast `ExtendedExplainInfo`'s coverage summary miscounted three kinds of node: - `ReusedSubqueryExec` fell through to the Spark bucket, adding a phantom un-accelerated operator at every reuse site. - `CometSubqueryBroadcastExec` did not mix in `CometPlan`, so Comet's own DPP operator was also counted as a Spark operator. - `ReusedExchangeExec` was both ignored by the match and unwrapped by `getActualPlan`, so the reused subtree was traversed and counted in full at every reference. On q14a that inflated the eligible count from 128 to 2302. Both reuse markers are now ignored and rendered as leaves, so the operators they reference are counted once, where the plan they point at is shown. `getActualPlan` keeps its existing unwrapping behaviour for fallback-reason collection; the coverage traversal uses the new `actualPlanForCoverage`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #5203.
Stacked on #5201 (
explain-expression-coverage) because that PR also regenerates everyTPC-DS
extended.txtgolden. Review that one first; the base will be retargeted tomainonce it merges.
Rationale for this change
The operator coverage summary produced by
ExtendedExplainInfocounted three node kindsincorrectly, so it understated acceleration on most TPC-DS plans and massively overcounted
operators wherever an exchange was reused. The same counts feed the
cometmetrics sourcevia
CometMetricsListener->CometSource, so the skew also biased theacceleration.ratiogauge.What changes are included in this PR?
All three items from the issue:
ReusedSubqueryExecis no longer counted as an un-accelerated Spark operator. It ispure reuse bookkeeping and a
LeafExecNode, so it now joins the ignore list.CometSubqueryBroadcastExecnow mixes inCometPlanand counts as accelerated. Theother
CometPlanmatch sites (CometExecRule,EliminateRedundantTransitions,foreachUntilCometInput,CometShuffleExchangeExec.isCometPlan) all reach nodes throughchildren, and aBaseSubqueryExechangs offinnerChildren/expressions, so none ofthem change behaviour. The one that would is
CometExecRule's "never replace" guard,which is the correct classification anyway.
getActualPlanunwrappedReusedExchangeExectoits child, so the reused subtree was traversed and counted in full at every reference. The
coverage traversal now uses a new
CometExplainInfo.actualPlanForCoverage, which isgetActualPlanminus that unwrapping. BecauseReusedExchangeExecis aLeafExecNode,keeping the wrapper renders a
ReusedExchangeleaf and stops the walk, matching howReusedSubqueryExecis handled and keeping the rendered tree consistent with the countsbelow it.
getActualPlanitself is unchanged, so fallback-reason collection stilldescends into reused subtrees.
Effect on the goldens: 143
extended.txtfiles change, and 107 of them now report 100%acceleration where only 6 did before. q1 is the example from the issue:
Eight plans report a lower percentage than before, which is the double counting being
removed: the reused subtrees were fully accelerated, so counting them once per reference
inflated the ratio. The clearest case is
approved-plans-v2_7/q14a, which went from2167 out of 2302to127 out of 128— that plan does not contain 2302 operators.Also documents in
understanding-comet-plans.mdwhich nodes are excluded from the counts.How are these changes tested?
New
CometCoverageStatsSuitecovers each of the three cases directly: aReusedSubquerycontributes nothing, a
ReusedExchangecontributes nothing at the reference site while theexchange it points at is still counted where it is defined, and wrapping a plan in
CometSubqueryBroadcastExecadds exactly one accelerated operator.TPC-DS plan stability goldens were regenerated for Spark 3.4, 3.5, 4.0, 4.1 and 4.2, and both
suites were then re-run in verification mode against the regenerated goldens on all five
versions (129 tests each, all passing).
CometExecSuite,CometCodegenSuiteand the newsuite also pass on the default profile (224 tests).